Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix track handles dissapearing #6338

Merged
merged 1 commit into from
Aug 16, 2024
Merged

Fix track handles dissapearing #6338

merged 1 commit into from
Aug 16, 2024

Conversation

allejok96
Copy link
Contributor

@allejok96 allejok96 commented Mar 22, 2022 •
edited
Loading

Fixes #6334, by letting Qt handle resizing of Song Editor's content.

I don't exactly understand what triggered the original bug, so testing is needed 😄

Also this PR removes unnecessary redrawing of ALL clips when:

  • Moving a track
  • Adding or deleting a track
  • Changing height of a track
  • Changing height of Song Editor
  • Opening/closing Song Editor

@LmmsBot
Copy link

LmmsBot commented Mar 22, 2022 •
edited
Loading

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Windows

Linux

macOS

🤖 
{"platform_name_to_artifacts": {"Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://output.circle-artifacts.com/output/job/f7421b7a-e1ac-415f-8d58-edbc1fef27c5/artifacts/0/lmms-1.3.0-alpha.1.181+g5f0df108a-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/16153?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://output.circle-artifacts.com/output/job/f45aca6f-6ce0-41c4-ba67-e7e24542cbd8/artifacts/0/lmms-1.3.0-alpha.1.181+g5f0df108a-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/16152?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/tj6h9uubsi24hkao/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/43202519"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/rqotn41tgdor36d2/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/43202519"}], "Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://output.circle-artifacts.com/output/job/3440e364-b2e6-4270-b130-a016324c26b7/artifacts/0/lmms-1.3.0-alpha.1.181+g5f0df108a-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/16149?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://output.circle-artifacts.com/output/job/f2382859-4ece-48f4-b347-604a4431631b/artifacts/0/lmms-1.3.0-alpha.1.181+g5f0df108a-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/16151?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "e3123e308fed97de36a65d94efb6a22f586be1a2"}

@Monospace-V
Copy link
Contributor

Having tested, I can't reproduce it with any conditions remotely similar to what I needed to reproduce it before.

@superpaik

This comment was marked as outdated.

Sign in to view
@Monospace-V

This comment was marked as outdated.

Sign in to view
@superpaik superpaik mentioned this pull request Mar 24, 2022
Closed
@superpaik

This comment was marked as outdated.

Sign in to view
@Spekular Spekular self-requested a review March 28, 2022 18:47
Spekular
Spekular approved these changes Mar 28, 2022
View reviewed changes
Copy link
Member

@Spekular Spekular left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neither the steps in #6334 nor #6330 reproduce the issue for me when testing this, code changes look sane.

@allejok96
Copy link
Contributor Author

I must investigate closer what happens when you update a TrackView and when it is needed. Perhaps renaming the method is stupid. At least there should be better comments.

@allejok96 allejok96 force-pushed the trackContainerResize branch from 19aab5f to e3123e3 Compare April 11, 2022 17:55
@allejok96
Copy link
Contributor Author

I've reverted the rename since I've discovered how much cleaning up is needed for the update() methods. That's for another time.

@allejok96 allejok96 requested a review from Spekular April 11, 2022 18:09
@Spekular
Copy link
Member

I've reverted the rename since I've discovered how much cleaning up is needed for the update() methods. That's for another time.

Just to double check, this was the only change made in the force push, right?

@allejok96
Copy link
Contributor Author

Correct

@PhysSong PhysSong linked an issue Apr 14, 2022 that may be closed by this pull request
Issues with track pointer movement w/ keyboard #6330
Open
@allejok96 allejok96 mentioned this pull request Apr 15, 2022
Open
@PhysSong PhysSong removed a link to an issue Apr 18, 2022
Issues with track pointer movement w/ keyboard #6330
Open
@allejok96 allejok96 force-pushed the trackContainerResize branch from e3123e3 to 06a2930 Compare February 27, 2023 17:44
@allejok96
Copy link
Contributor Author

Hey, I've reverted to the bare minimum needed to fix the bug. Ready to merge when tested @Veratil

I'll make another PR with the redraw related fixes because I have found more of them...

@Veratil
Copy link
Contributor

Veratil commented Feb 27, 2023

@Monospace-V @PhysSong can you also test that this fixes the issue and doesn't cause any regression of behavior by removing the resizeEvent function?

@Rossmaxx
Copy link
Contributor

Update about the fixes/any regressions? Is this ready for merge?

This was referenced May 22, 2024
Closed
Closed
@Rossmaxx
Copy link
Contributor

Looking at the code, there doesn't seem to be an issue from removing resizeEvent overload, other than that there is a call to realignTracks (but why?). I'll hit merge soon after someone confirms.

@M4rotte
Copy link

M4rotte commented Aug 16, 2024 •
edited
Loading

Looking at the code, there doesn't seem to be an issue from removing resizeEvent overload, other than that there is a call to realignTracks (but why?). I'll hit merge soon after someone confirms.

I merged this commit on the very last lmms/master branch (bda1a9c) and noticed no problem either.

There is still the fact that having a sample track with a sample in it, makes the whole song editor very slow to response, it makes it stutters if I may say (the display, not the audio hopefully!). But I noticed that some time ago, it’s not aggravated by the commit anyhow.

I use sample tracks in the song editor nearly never myself, not even for this slowness, just a matter of taste I guess.

@Rossmaxx
Copy link
Contributor

There is still the fact that having a sample track with a sample in it, makes the whole song editor very slow to response, it makes it stutters if I may say

That's a completely different issue. That's due to sample graph rendering.

Since i got a confirmation, merging.

@Rossmaxx Rossmaxx merged commit 58ce9b4 into LMMS:master Aug 16, 2024
mmeeaallyynn pushed a commit to mmeeaallyynn/lmms that referenced this pull request Oct 27, 2024
@allejok96 @mmeeaallyynn
Fix track handles dissapearing (LMMS#6338)
a591a4b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rossmaxx Rossmaxx Rossmaxx approved these changes

@Spekular Spekular Spekular approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Drag handle disappearance
8 participants
@allejok96 @LmmsBot @Monospace-V @superpaik @Spekular @Veratil @Rossmaxx @M4rotte